Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pkg: validate: validate Username not empty in ImageStatus #250

Merged
merged 1 commit into from
Feb 24, 2018
Merged

pkg: validate: validate Username not empty in ImageStatus #250

merged 1 commit into from
Feb 24, 2018

Conversation

runcom
Copy link
Contributor

@runcom runcom commented Feb 14, 2018

Kubernetes rely on that Username field to provide RunAsUser, we need to
validate runtimes correctly return it. We had recently an issue in
CRI-O for that.

@feiskyer @mrunalp PTAL

Signed-off-by: Antonio Murdaca [email protected]

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2018
@mrunalp mrunalp mentioned this pull request Feb 14, 2018
Closed
@mrunalp
Copy link
Member

mrunalp commented Feb 14, 2018

👍

@runcom runcom force-pushed the validate-image-status-username branch 2 times, most recently from 33877da to c817f49 Compare February 15, 2018 18:21
@runcom
Copy link
Contributor Author

runcom commented Feb 16, 2018

@feiskyer @Random-Liu do you know why the test pull the image but show an empty Username field in status?

@feiskyer
Copy link
Member

@runcom 1002 is actually a Uid instead of username, could we change the validation to check Uid instead?

@runcom
Copy link
Contributor Author

runcom commented Feb 16, 2018

Yup, blame on me

@runcom
Copy link
Contributor Author

runcom commented Feb 16, 2018

We need a test for Username as well, I'll add one

@runcom
Copy link
Contributor Author

runcom commented Feb 16, 2018

@feiskyer actually, there's no UID field in image status, that's why I was asking

@feiskyer
Copy link
Member

feiskyer commented Feb 16, 2018 •
edited
Loading

@runcom There are both Uid and Username, See https://github.com/kubernetes/kubernetes/blob/ada9400915e2828fdf1c22f1723d5fcc345180d2/pkg/kubelet/apis/cri/runtime/v1alpha2/api.proto#L972-L978

@runcom runcom force-pushed the validate-image-status-username branch from c817f49 to c5e0aa9 Compare February 16, 2018 14:14
@runcom
Copy link
Contributor Author

runcom commented Feb 16, 2018

blame on me again, I've fixed the PR, thanks!

@runcom runcom force-pushed the validate-image-status-username branch 3 times, most recently from 9713118 to 23d2b64 Compare February 16, 2018 15:48
@runcom
Copy link
Contributor Author

runcom commented Feb 16, 2018

ready now

@Random-Liu
Copy link
Contributor

Can we put the Dockerfile into this repo? I can build one and push to gcr kubernetes test image repo.

@runcom
Copy link
Contributor Author

runcom commented Feb 17, 2018

Yup, I can do it, can we do the same for the PR I've opened in kubernetes/kubernetes as well? @Random-Liu

@Random-Liu Random-Liu self-assigned this Feb 17, 2018
@runcom runcom force-pushed the validate-image-status-username branch from 23d2b64 to 718162d Compare February 17, 2018 12:22
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 17, 2018
@runcom
Copy link
Contributor Author

runcom commented Feb 17, 2018

@Random-Liu this PR now contains your PR #252 - I've added the Dockerfile under images/image-user/.... When that's merged,I'll rebase this one

@Random-Liu Random-Liu added this to the v1.0.0-beta.0 milestone Feb 21, 2018
@Random-Liu
Copy link
Contributor

#252 is merged. Please update your PR and I'll review. :)

@runcom runcom force-pushed the validate-image-status-username branch from 718162d to 6445f59 Compare February 22, 2018 11:51
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 22, 2018
@runcom runcom force-pushed the validate-image-status-username branch 2 times, most recently from fdeed0e to ef01abf Compare February 22, 2018 12:03
@runcom
Copy link
Contributor Author

runcom commented Feb 22, 2018

@Random-Liu updated, please push test images on gcr.io or otherwise this will fail :)

Random-Liu
Random-Liu reviewed Feb 22, 2018
View reviewed changes
images/image-user/Makefile
@@ -0,0 +1,24 @@
# Copyright 2018 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got this:

Makefile:23: *** non-numeric first argument to 'word' function: '{1..4}'.  Stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed now.

@runcom runcom force-pushed the validate-image-status-username branch from ef01abf to 98fcad9 Compare February 23, 2018 13:57
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2018
@runcom
Copy link
Contributor Author

runcom commented Feb 23, 2018

@Random-Liu PTAL again please

Random-Liu
Random-Liu reviewed Feb 23, 2018
View reviewed changes
pkg/validate/image.go Outdated
@@ -36,6 +37,15 @@ const (

// digested reference for test image
testImageWithDigest = "gcr.io/cri-tools/test-image-digest@sha256:9179135b4b4cc5a8721e09379244807553c318d92fa3111a65133241551ca343"

testImageUserUID = "gcr.io/cri-tools/test-user-uid"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/test-user-uid/test-image-user-uid
and also the following images.

pkg/validate/image.go Outdated
},
{
description: "Username only",
image: testImageUserUID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testImageUserUsername

@Random-Liu
Copy link
Contributor

@runcom Image pushed.

@runcom
pkg: validate: validate Username not empty in ImageStatus
e082452 
Kubernetes rely on that Username field to provide RunAsUser, we need to
validate runtimes correctly return it. We had recently an issue in
CRI-O for that.

Signed-off-by: Antonio Murdaca <[email protected]>
@runcom runcom force-pushed the validate-image-status-username branch from 98fcad9 to e082452 Compare February 24, 2018 11:03
@runcom
Copy link
Contributor Author

runcom commented Feb 24, 2018

@Random-Liu took care of your comments and this is now green

feiskyer
feiskyer approved these changes Feb 24, 2018
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2018
@feiskyer feiskyer merged commit b184f9a into kubernetes-sigs:master Feb 24, 2018
@runcom runcom deleted the validate-image-status-username branch February 24, 2018 15:43
@Random-Liu Random-Liu mentioned this pull request Feb 25, 2018
Merged
@feiskyer feiskyer mentioned this pull request Feb 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Random-Liu Random-Liu Random-Liu left review comments

@feiskyer feiskyer feiskyer approved these changes

Assignees

@feiskyer feiskyer

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.0.0-beta.0
Development

Successfully merging this pull request may close these issues.
5 participants
@runcom @mrunalp @feiskyer @Random-Liu @k8s-ci-robot